Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.69%. Comparing base (4f67189) to head (899a5c4).
Report is 8 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2060      +/-   ##
==========================================
+ Coverage   83.67%   83.69%   +0.01%     
==========================================
  Files         312      312              
  Lines        7106     7114       +8     
  Branches     1054     1054              
==========================================
+ Hits         5946     5954       +8     
  Misses        789      789              
  Partials      371      371              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@martintmk Is there any particular reason why the RetryAfter is not passed to the OnRateLimiterRejectedArguments?

https://github.com/App-vNext/Polly/blob/main/src/Polly.RateLimiting/RateLimiterResilienceStrategy.cs#L41

TimeSpan? retryAfter = null;

if (lease.TryGetMetadata(MetadataName.RetryAfter, out TimeSpan retryAfterValue))
{
    retryAfter = retryAfterValue;
}

var args = new OnRateLimiterRejectedArguments(context, lease);
_telemetry.Report(new(ResilienceEventSeverity.Error, RateLimiterConstants.OnRateLimiterRejectedEvent), context, args);

if (OnLeaseRejected != null)
{
    await OnLeaseRejected(new OnRateLimiterRejectedArguments(context, lease)).ConfigureAwait(context.ContinueOnCapturedContext);
}

var exception = retryAfter.HasValue ? new RateLimiterRejectedException(retryAfter.Value) : new RateLimiterRejectedException();

return Outcome.FromException<TResult>(exception.TrySetStackTrace());

Is there any particular reason why the RetryAfter is not passed to the OnRateLimiterRejectedArguments?

Frankly, I don't know :D

It was probably just an omission as we didn't have any use-case where this would be required. If there is a real-world use-case we can add it there.

One thing I am wondering is to add Telemetry section with the name of the events that each resilience strategy produces. It can also point to proper telemetry page in case someone needs more details.

https://www.pollydocs.org/advanced/telemetry.html#metrics

One thing I am wondering is to add Telemetry section with the name of the events that each resilience strategy produces. It can also point to proper telemetry page in case someone needs more details.

https://www.pollydocs.org/advanced/telemetry.html#metrics

Good idea, I'll do that.

@martintmk Do I see it correctly that this transitionedState variable can be removed from here?

https://github.com/App-vNext/Polly/blob/main/src/Polly.Core/CircuitBreaker/Controller/CircuitStateController.cs#L337

Do I see it correctly that this transitionedState variable can be removed from here?

Looks like some leftovers from a refactor to me.

@martincostello , @martintmk How do you like it?

@martintmk I've just realized that the Execution Time is reported without the unit in case of Execution Attempt.

I know it is mentioned in the documentation that the unit is millisecond, but I would rather prefer self-explanatory events.

Expected

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1',
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839 ms'

Actual

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1', 
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839'

Same applies for PipelineExecuting and PipelineExecuted events.

Can we fix these in a separate PR?

@martintmk I've just realized that the Execution Time is reported without the unit in case of Execution Attempt.

I know it is mentioned in the documentation that the unit is millisecond, but I would rather prefer self-explanatory events.

Expected

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1',
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839 ms'

Actual

Execution attempt. Source: '(null)/(null)/Hedging', Operation Key: '', Result: '1', 
 Handled: 'False', Attempt: '0', Execution Time: '1505.3839'

Same applies for PipelineExecuting and PipelineExecuted events.

Can we fix these in a separate PR?

Sure, makes sense. Just don't put the ms formatting string so it isn't included in value when structured logging is used.